[KeyVault] - Migrate KeyVault Certificates, Keys and Secrets to Core V2 by JonathanCrd · Pull Request #21702 · Azure/azure-sdk-for-js

JonathanCrd · 2022-04-29T23:36:11Z

Packages impacted by this PR

KeyVault Certificates
KeyVault Keys
KeyVault Secrets
KeyVault Common (Not a published package)
KeyVault Admin

Issues associated with this PR

Describe the problem that is addressed by this PR

This PR finishing updating all of KeyVault to use CoreV2 and removes all their dependencies to the old core. For more information about this type of migrations refer to this wiki.

Since these 3 packages depends on common, they needed to be migrated at the same time. The following is a summary of the main changes:

Dependencies and generation changes:

Updated dependencies in package jsons.
Removed all instances and import from core-http and replaced them with their core-client, core-rest-pipeline and core-auth equivalent.
Regenerated clients with latest version of autorest and v2 flag set to true.
isNode and delay are now imported from core-utils preview, since Admin client already depends on it.
Added core-http-compat as a dependency to use ExtendedCommonClientOptions in all packages instead of CommonClientOptions from core-client.

Authentication changes:

Challenge Based Auth removed from Admin and moved to Common.
Removed SigningPolic from all packages.
Using bearerTokenAuthenticationPolicy to authenticate.
Removed isTokenCredential from imports.
Added an auth scope in constants.ts
Challenge authentication was not properly getting the auth scope from the server response, this was a bug that got fixed in this PR. Now the scope is gathered from the parsedChallenge.resource.

Convenience layer changes:

Inside listing or pagination methods, updated operation calls that passes a continuation token to use the new generated methods that has the Next keyword in them. For example: getDeletedSecret(continuationToken, options ) to getDeletedSecretNext(vaultURI, continuationToken, options).

Test changes:

Updated the recordings.
Added the used of an XHR Http Client in TestAuthentication.ts
Did required fixes for test that stop passing after the migration
Challenge auth test were removed from Secrets, Certificates and Keys and left on Admin, since all of the KeyVault packages run on CI.

Recorder updates:

KeyVault Keys has been upgraded to use the new recorder.

All of the packages build correctly, and tests pass as well.

Related PRs

[KeyVault] - Migrate Key Vault Admin package to Core V2 #15881

Checklists

Added impacted package name to the issue description
Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
Added a changelog (if necessary)

…tificates

JonathanCrd · 2022-06-09T21:32:43Z

I have updated the PR description with the latest changes

…tificates

deyaaeldeen · 2022-06-10T22:04:53Z

@@ -260,32 +260,34 @@ export interface ErrorModel {
    readonly message?: string;
 }

+export { ExtendedCommonClientOptions }


why is it re-exported?

I'm not sure, the only place where this is being exported is in index.ts on line 197.

CommonClientOptions from Core-http was also being exported there, I just renamed it to the equivalent in CoreV2.

This change is breaking then but I wonder what was the point of re-exporting the old interface in the first place. I am thinking of biting the bullet and removing the re-export even though that is also a breaking change. @xirzec what do you think?

For the record, PipelineOptions from core-http (which has been removed in this PR) was being exported too in the same way as ExtendedCommonClientOptions.
You can find it here:
https://github.com/JonathanCrd/azure-sdk-for-js/blob/main/sdk/keyvault/keyvault-certificates/review/keyvault-certificates.api.md?plain=1#L433

I'm also curious why this behavior, however, wouldn't it be a breaking change anyway since PipelineOptions is no longer being exported?

I would like to get this PR merged in the coming days, but want to resolve this first. I did a bit of digging to try and understand the impact of the change. PipelineOptions was initially exported in this commit from 3 years back and hasn't been touched since. At the time of that commit, the CertificateClient constructor took PipelineOptions as a parameter, so you can see why the export was required then. This was extracted out to a CertificateClientOptions by Daniel here, which is what we have now.

I don't see the point in exporting the new ExtendedCommonClientOptions as is here -- let's remove that at least. If we were to remove the export of PipelineOptions, would we still be able to release this as a minor? I can think of some hackery that might be less breaking (re-exporting ExtendedCommonClientOptions as PipelineOptions...) but that doesn't sit very well with me since we're effectively giving the type an incorrect name.

Thoughts, @deyaaeldeen, @xirzec, @joheredi (and anybody else)?

To wrap this up: After an offline discussion, we're in agreement it would be ok to remove the export of PipelineOptions and still release this as a minor.

Cc @deyaaeldeen @joheredi @bterlson

timovv

🚀

JonathanCrd added 30 commits April 5, 2022 14:30

Create local copy of IsNode

6ff01a4

Update swagger config file and package json

ac194c3

Import IsNode from local copy

f4eedc5

Regen client

6bfef29

Changes in custom layer

b46857a

Regen Certificates Client

bbe22ac

Use latest version of autorest.typescript

90a8af9

Update LRO Certificates

4ff44a6

Regenerate KV Certificates to fix nullable issue

64a5b1d

Update Certificates Custom Layer

1e6f384

Update tests for KV Certificates

fd1c368

Migrate KV Common to CoreV2 and add extra utils

3467ff3

Build KV Certificates

e4e15b3

Update dependencies and generate client

0c35da3

Update KV Keys src

65540a4

Update KV Keys Cryptography

d12e533

Update KV Keys LRO

5247665

Update KV Keys Internal Tests

ff4eefc

Update KV Keys Public Utils Tests

9c4c5a6

Update KV Keys public tests

adbdf44

Fix build error and rebuild KV Keys

59f04bf

Adding browser mapping to KV common

900c64a

KV Secrets Update Package Version

c472bed

KV Secrets Regenerate Client

1f04835

KV Secrets Update src

3cdb6af

KV Secrets remove context file (no longer generated)

ef20365

KV Secrets Update tests

2f615b1

KV Secrets build package

e364e75

Update KV Secrets constants

b742e57

Import IsNode and delay from core-util

1cce6ea

JonathanCrd added 3 commits June 9, 2022 01:00

Merge remote-tracking branch 'upstream/main' into CoreV2-KeyVault-Cer…

878fb94

…tificates

Rush update

71766f3

Rebuild packages

8769bcc

JonathanCrd marked this pull request as ready for review June 9, 2022 19:56

JonathanCrd requested review from bterlson, deyaaeldeen, jeremymeng, praveenkuttappan, timovv, witemple-msft and xirzec as code owners June 9, 2022 19:56

timovv reviewed Jun 9, 2022

View reviewed changes

timovv reviewed Jun 10, 2022

View reviewed changes

deyaaeldeen reviewed Jun 10, 2022

View reviewed changes

Comment thread sdk/keyvault/keyvault-admin/src/accessControlClient.ts

JonathanCrd added 2 commits June 10, 2022 15:00

Addressing comments

4f40171

Merge remote-tracking branch 'upstream/main' into CoreV2-KeyVault-Cer…

0887e34

…tificates

deyaaeldeen reviewed Jun 10, 2022

View reviewed changes

JonathanCrd added 2 commits June 10, 2022 15:12

Update pnpm-lock.yaml

9c419e9

Add Challenge Authentication tests in all packages

bba9952

deyaaeldeen reviewed Jun 10, 2022

View reviewed changes

Comment thread sdk/keyvault/keyvault-certificates/swagger/README.md

deyaaeldeen reviewed Jun 10, 2022

View reviewed changes

Comment thread sdk/keyvault/keyvault-secrets/package.json Outdated

Depend on GA version of @azure/core-util

f6e65aa

JonathanCrd assigned timovv Jun 14, 2022

timovv added 2 commits June 14, 2022 14:26

Merge branch 'main' into CoreV2-KeyVault-Certificates

60e9911

Remove ExtendedCommonClientOptions exports from all packages

70ef94c

timovv approved these changes Jul 6, 2022

View reviewed changes

timovv merged commit 8f72772 into Azure:main Jul 6, 2022

timovv mentioned this pull request Aug 26, 2022

Key Vault Keys - Move to the new proxy server based recorder #18004

Closed

timovv mentioned this pull request Oct 11, 2022

Move remainder of Key Vault packages to core-rest-pipeline #15896

Closed

3 tasks

Conversation

JonathanCrd commented Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Packages impacted by this PR

Issues associated with this PR

Describe the problem that is addressed by this PR

Dependencies and generation changes:

Authentication changes:

Convenience layer changes:

Test changes:

Recorder updates:

Related PRs

Checklists

Uh oh!

JonathanCrd commented Jun 9, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deyaaeldeen Jun 10, 2022

Choose a reason for hiding this comment

Uh oh!

JonathanCrd Jun 10, 2022

Choose a reason for hiding this comment

Uh oh!

deyaaeldeen Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonathanCrd Jun 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timovv Jun 23, 2022

Choose a reason for hiding this comment

Uh oh!

timovv Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

timovv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JonathanCrd commented Apr 29, 2022 •

edited

Loading

deyaaeldeen Jun 10, 2022 •

edited

Loading

JonathanCrd Jun 11, 2022 •

edited

Loading